Summary

Solid, well-scoped fix for a real problem: a broken native dep under sentence-transformers (torchcodec/FFmpeg) or an arch-mismatched faiss used to crash every agent at import time, even when RAG was never touched. Widening the guard from except ImportError to except Exception and deferring the loud error to _check_dependencies() is the right shape, and the comments explain the why clearly.

One blocking issue: the secondary goal — telling "not installed" apart from "installed but broken" — doesn't actually work for the common case. Because the guard captures ModuleNotFoundError (a subclass of ImportError) into _*_IMPORT_ERROR too, a genuinely missing package gets reported under the "installed but failed to load — reinstalling won't help" banner, which is the exact misdirection the PR set out to fix. The new test passes only because it hand-sets the error to None rather than exercising the real import path.

Issues Found

🟡 Important — genuinely-missing dep is mislabeled "installed but failed to load" (src/gaia/rag/sdk.py:41-45)

except Exception catches ModuleNotFoundError (an ImportError), so when a package is simply not installed, _*_IMPORT_ERROR is still truthy. _check_dependencies (sdk.py:242-247) then adds it to broken and prints "installed but failed to load — reinstalling won't help until the underlying error is fixed" — telling a user whose fix is pip install not to bother. That's the most common path (anyone without the [rag] extras who triggers RAG), and it directly contradicts the PR's stated purpose.

Fix: only capture the cause when it is not an ImportError. For the sentence-transformers block:

_SENTENCE_TRANSFORMERS_IMPORT_ERROR = None
try:
    from sentence_transformers import SentenceTransformer
except ImportError:
    SentenceTransformer = None
except Exception as _e:  # pylint: disable=broad-except
    SentenceTransformer = None
    _SENTENCE_TRANSFORMERS_IMPORT_ERROR = _e

And the same shape for the faiss block (sdk.py:24-30):

_FAISS_IMPORT_ERROR = None
try:
    import faiss
except ImportError:
    faiss = None
except Exception as _e:  # pylint: disable=broad-except
    faiss = None
    _FAISS_IMPORT_ERROR = _e

With this, genuinely-missing → _*_IMPORT_ERROR stays None → the broken-section is omitted (which is exactly what test_genuinely_missing_dep_omits_broken_section simulates), and only true native-load failures (RuntimeError/OSError) get the "reinstalling won't help" hint.

🟢 Minor — "genuinely missing" test doesn't exercise the real capture path (tests/unit/rag/test_dependency_guard.py:112)

test_genuinely_missing_dep_omits_broken_section manually patches _SENTENCE_TRANSFORMERS_IMPORT_ERROR to None, which is not the state the import block produces for a missing package (it produces a ModuleNotFoundError). That's why the test stays green despite the bug above. After applying the import fix, consider also asserting the import block itself yields None for an ImportError cause (e.g. simulate by re-running the try/except logic with a patched importer), so the regression is actually pinned rather than assumed.

Strengths

• Correct diagnosis and minimal blast radius — the import-crash fix itself is sound: import succeeds, agents load, and the error is deferred to point-of-use in __init__ via _check_dependencies().
• The except Exception here is justified (KeyboardInterrupt/SystemExit derive from BaseException and stay uncaught), and the pylint: disable is appropriately narrow.
• Comments explain the why (native-load failures raise RuntimeError/OSError, not ImportError) in one tight block rather than narrating the what — matches the repo's comment guidance.
• Test file is focused and readable, and the module-imports-without-deps test directly pins the regression that matters most.

Verdict

Request changes — the core import-crash fix is good to go, but the missing-vs-broken distinction misfires on the common "not installed" path and prints actively misleading guidance. The fix is two small per-import edits (above) plus tightening the one test; happy to re-approve once applied.

🟡 tests/unit/rag/test_dependency_guard.py — tests patch the wrong hook and don't exercise the code they claim to cover.

_check_dependencies() calls importlib.import_module(pkg), but both new tests intercept builtins.__import__. Those two entry points are independent: importlib.import_module goes straight to _bootstrap._gcd_import and never touches builtins.__import__. The fake RuntimeError/ImportError is never raised, broken stays empty, and test_broken_install_reports_actionable_cause will fail on any CI node where sentence-transformers isn't installed (the assert "installed but failed to load" in msg assertion won't be satisfied).

Fix: patch sdk.importlib.import_module instead of builtins.__import__:

    def fake_import(name, *args, **kwargs):
        if name == "sentence_transformers":
            raise RuntimeError("Could not load libtorchcodec (FFmpeg not found)")
        return real_import_module(name)

    real_import_module = importlib.import_module
    monkeypatch.setattr(sdk.importlib, "import_module", fake_import)

(Same pattern for the test_genuinely_missing_dep_omits_broken_section test — swap builtins.__import__ for sdk.importlib.import_module.)